Validator manager commands for the Keymanager APIs #6261
Conversation
…anager-standard-keystore
michaelsproul
added
ready-for-review
There was a problem hiding this comment.
Thank you for your review and help @macladson and @michaelsproul. I added some comments to some parts that I am not sure. The import command now supports importing keystore JSON files created using:
lighthouse vm createby using the commandlighthouse vm import- staking-deposit-cli or ethstaker-deposit-cli using
lighthouse vm import --standard-format
@macladson mentioned about supporting multiple validators with the standard format, I will look into this; in the mean time, this is ready for a re-review.
Thanks!
| fee_recipient: None, | ||
| builder_boost_factor: None, | ||
| gas_limit: None, | ||
| builder_proposals: Some(false), |
There was a problem hiding this comment.
I have to let this to be Some(false) for the comparison to be equal. Not sure if this is ok?
The same goes for prefer_builder_proposals field
There was a problem hiding this comment.
The reason is because of here.
We set it in the config directly as Some(get_flag) so in the case the flag is missing it will be set to Some(false).
Unless I'm missing something, I think this should probably just be a parse_optional like the other fields in the config.
| Keystore::from_json_file(&validators_file_path).map_err(|e| format!("{e:?}"))?, | ||
| ), | ||
| voting_keystore_password: password.ok_or_else(|| { | ||
| "The --password flag is required to supply the keystore password".to_string() |
There was a problem hiding this comment.
I have to put ok_or_else here to handle the None case (when --password flag) is not provided. We already have requires here, so not sure about this here, or under what condition will this manifest?
There was a problem hiding this comment.
Because we want to support both formats, we have to have password be an Option. Then since we want to build the ImportConfig before we know which format we are using, we are stuck with password being an Option.
The or_else should never trigger since the require will trigger before we even get here, but this pattern is fine as is imo. We have redundant patterns like this in plenty of places.
| fee_recipient: None, | ||
| builder_boost_factor: None, | ||
| gas_limit: None, | ||
| builder_proposals: Some(false), |
There was a problem hiding this comment.
The reason is because of here.
We set it in the config directly as Some(get_flag) so in the case the flag is missing it will be set to Some(false).
Unless I'm missing something, I think this should probably just be a parse_optional like the other fields in the config.
| @@ -80,14 +83,94 @@ pub fn cli_app() -> Command { | |||
| ) | |||
| .display_order(0), | |||
| ) | |||
| .arg( | |||
| Arg::new(PASSWORD) | |||
There was a problem hiding this comment.
Is the --password flag only valid when using --standard-format? If that's the case, we should add a .requires(STANDARD_FORMAT) to its cli definition. This is kind of circular but I think it should be fine. Without this, a user would be able to use the --password flag when using the Lighthouse format which would be silently ignored.
| Keystore::from_json_file(&validators_file_path).map_err(|e| format!("{e:?}"))?, | ||
| ), | ||
| voting_keystore_password: password.ok_or_else(|| { | ||
| "The --password flag is required to supply the keystore password".to_string() |
There was a problem hiding this comment.
Because we want to support both formats, we have to have password be an Option. Then since we want to build the ImportConfig before we know which format we are using, we are stuck with password being an Option.
The or_else should never trigger since the require will trigger before we even get here, but this pattern is fine as is imo. We have redundant patterns like this in plenty of places.
Issue Addressed
Proposed Changes
Add commonds to
lighthouse vmto support the Keymanager API to manage local keystores, i.e.,list,importanddeleteAdditional Info
Building on top of @pahor167 PR: #5347
Some other modifications:
importlogging where only successful import will display the log (previously it will display the log once the HTTP API is called even though the import is not successful, e.g., incorrect password)delete